test: scoring.ts, rule-engine.ts 핵심 모듈 유닛 테스트 추가#58
Merged
Conversation
scoring.ts had zero tests despite being the core scoring output module. rule-engine.test.ts had only 1 test (state isolation). This adds: scoring.test.ts (23 tests): - calculateScores: zero issues, severity counting, density weights, diversity penalty, combined formula, floor clamping, nodeCount edge cases - Grade boundaries: S at 100%, F below 50%, gradeToClassName - formatScoreSummary, getCategoryLabel, getSeverityLabel - buildResultJson field validation rule-engine.test.ts (+15 tests): - targetNodeId: subtree scoping, dash-to-colon normalization, missing node error - excludeNodeTypes / excludeNodeNames filtering - enabledRules / disabledRules / config.enabled filtering - depthWeight interpolation (root vs leaf scoring) - Tree traversal: nodeCount, maxDepth, empty document - Error resilience: analysis continues when rules throw - analyzeFile convenience function with custom configs Closes #55 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds comprehensive unit tests for the core analysis engine and scoring logic: new suites validate RuleEngine.analyze/analyzeFile behaviors (filtering, subtree selection, error resilience) and scoring utilities (density/diversity/severity weighting, grading, and presentation helpers). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/engine/rule-engine.test.ts`:
- Around line 258-286: The test's main assertion is guarded by an if and can
pass vacuously if issues aren't found; update the test around analyzeFile/result
to assert presence of the expected issues before comparing scores by adding
explicit assertions like expect(issueAtChild).toBeDefined() and
expect(issueAtGC).toBeDefined() (or remove the conditional and let the expect
calls fail) so the comparison of issueAtChild.calculatedScore and
issueAtGC.calculatedScore runs only when both issues are present; locate the
variables issueAtChild, issueAtGC and the analyzeFile call to apply this change.
- Around line 359-382: The test currently doesn't inject an error — add a rule
that throws inside its check() and ensure analyzeFile(file) still completes:
create a mock rule (e.g., { id: "thrower", check: () => { throw new
Error("boom") }, applicable: () => true }) and register or inject it into the
RuleEngine/rules collection used by analyzeFile (or spy/mock an existing rule's
check method to throw), then call analyzeFile(file), assert result.issues and
result.nodeCount as before, and keep the console.error spy to verify the thrown
error was logged without halting analysis.
In `@src/core/engine/scoring.test.ts`:
- Around line 270-288: The helper function scoreWithPercentage is dead code
(defined but never used); remove the unused function scoreWithPercentage from
src/core/engine/scoring.test.ts, or alternatively implement the intended
parameterized tests that call scoreWithPercentage with target percentages and
assert expected grades (use existing helpers makeResult and calculateScores to
produce results and verify grade boundaries). Ensure any added tests reference
scoreWithPercentage, makeResult, and calculateScores so the helper is actually
exercised; otherwise delete the helper to keep the test file clean.
- Around line 295-319: Replace the non-null assertion on rulesPerCat[cat] in the
nested loop with an explicit safe lookup: retrieve const rules =
rulesPerCat[cat] and skip/continue if rules is undefined before iterating, then
iterate over rules instead; this removes the `!` assertion while keeping the
same behavior and references the existing symbols rulesPerCat, categories, and
the nested for (const ruleId ...) loop in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d8aa6f2e-bfbc-4729-a53d-421a69b72c0a
📒 Files selected for processing (2)
src/core/engine/rule-engine.test.tssrc/core/engine/scoring.test.ts
- Remove dead code: unused scoreWithPercentage helper in scoring.test.ts - Replace non-null assertion (rulesPerCat[cat]!) with safe lookup + continue - Add explicit toBeDefined() assertions for depthWeight test to prevent vacuous pass; fix test tree so grandchild FRAME has children (required by no-auto-layout rule) - Inject actual error in resilience test: spy on default-name rule's check to throw, verify console.error is called, restore original Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/core/engine/rule-engine.test.ts (1)
160-164:⚠️ Potential issue | 🟡 MinorStrengthen filtering tests to avoid vacuous passes.
Both tests only assert filtered output when baseline issues exist, so they can pass without validating the filter behavior. Fail fast on baseline presence, then assert exclusion.
Suggested test hardening
@@ - expect(textIssuesExcluded.length).toBeLessThanOrEqual(textIssuesAll.length); - // If TEXT generated any issues, they should be filtered out - if (textIssuesAll.length > 0) { - expect(textIssuesExcluded.length).toBe(0); - } + expect(textIssuesAll.length).toBeGreaterThan(0); + expect(textIssuesExcluded.length).toBe(0); @@ - // default-name issues should be absent when disabled - if (defaultNameAll.length > 0) { - expect(defaultNameDisabled.length).toBe(0); - } + expect(defaultNameAll.length).toBeGreaterThan(0); + expect(defaultNameDisabled.length).toBe(0);Also applies to: 232-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/engine/rule-engine.test.ts` around lines 160 - 164, The tests currently only assert that textIssuesExcluded.length <= textIssuesAll.length and conditionally check exclusion, which allows vacuous passes; change the assertions to first fail fast if the baseline has no issues (e.g., require textIssuesAll.length > 0 using expect(textIssuesAll.length).toBeGreaterThan(0)), then assert the exclusion behavior (expect(textIssuesExcluded.length).toBe(0)); apply the same pattern to the duplicate block that uses the other pair of variables at 232-234 so the filter behavior is actually validated when a baseline exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/engine/rule-engine.test.ts`:
- Around line 160-164: The tests currently only assert that
textIssuesExcluded.length <= textIssuesAll.length and conditionally check
exclusion, which allows vacuous passes; change the assertions to first fail fast
if the baseline has no issues (e.g., require textIssuesAll.length > 0 using
expect(textIssuesAll.length).toBeGreaterThan(0)), then assert the exclusion
behavior (expect(textIssuesExcluded.length).toBe(0)); apply the same pattern to
the duplicate block that uses the other pair of variables at 232-234 so the
filter behavior is actually validated when a baseline exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2df8a647-f0c6-47d4-9021-963cc99dead8
📒 Files selected for processing (2)
src/core/engine/rule-engine.test.tssrc/core/engine/scoring.test.ts
Replace conditional assertions (if baseline > 0) with explicit baseline assertions (expect > 0) in excludeNodeTypes and disabledRules tests, ensuring the filter behavior is always validated. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/core/engine/rule-engine.test.ts (2)
165-182:⚠️ Potential issue | 🟠 Major
excludeNodeNamestest does not prove exclusion behavior.This only asserts
0after filtering; it never proves the ignored node produced issues before filtering. Add a baseline run and assert those issues exist first.Proposed fix
it("skips nodes matching excludeNodeNames pattern", () => { @@ const file = makeFile({ document: doc }); - const result = analyzeFile(file, { excludeNodeNames: ["IgnoreMe"] }); + const baseline = analyzeFile(file); + const result = analyzeFile(file, { excludeNodeNames: ["IgnoreMe"] }); + + const ignoredIssuesBaseline = baseline.issues.filter( + (i) => i.violation.nodeId === "i:1" + ); const ignoredIssues = result.issues.filter( (i) => i.violation.nodeId === "i:1" ); + expect(ignoredIssuesBaseline.length).toBeGreaterThan(0); expect(ignoredIssues.length).toBe(0); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/engine/rule-engine.test.ts` around lines 165 - 182, The test "skips nodes matching excludeNodeNames pattern" doesn't prove exclusion because it never verifies the ignored node produced issues before applying excludeNodeNames; update the test to first run analyzeFile(file) without the exclude option and assert there are one or more issues for node id "i:1" (using makeNode/makeFile and result.issues filtering), then run analyzeFile(file, { excludeNodeNames: ["IgnoreMe"] }) and assert the filtered ignoredIssues length is 0 to demonstrate the exclusion behavior.
188-206:⚠️ Potential issue | 🟠 Major
enabledRulestest can pass vacuously with zero issues.If analysis returns no issues, the loop never asserts anything. Add a non-empty guard so the test actually validates rule filtering.
Proposed fix
it("runs only enabledRules when specified", () => { @@ const result = analyzeFile(file, { enabledRules: ["default-name"] }); + // Ensure this test cannot pass vacuously + expect(result.issues.length).toBeGreaterThan(0); + // All issues should be from default-name only for (const issue of result.issues) { expect(issue.violation.ruleId).toBe("default-name"); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/engine/rule-engine.test.ts` around lines 188 - 206, The test "runs only enabledRules when specified" can pass vacuously when result.issues is empty; update the assertion to first assert there is at least one issue (e.g., expect(result.issues.length).toBeGreaterThan(0) or similar) before iterating, then keep the existing loop that ensures every issue.violation.ruleId === "default-name" using analyzeFile, enabledRules and result.issues to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/engine/rule-engine.test.ts`:
- Around line 234-251: Update the test "skips rules disabled in config (enabled:
false)" to include a positive control: first run analyzeFile with the same
fixture but with the "no-dev-status" rule explicitly enabled (e.g., by passing a
config override or setting the analyzer options) and assert that result.issues
contains at least one issue whose violation.ruleId === "no-dev-status"; then run
the existing default analyzeFile call to assert zero issues. Reference the
existing test name and the analyzeFile function and the rule id "no-dev-status"
when making the change.
---
Duplicate comments:
In `@src/core/engine/rule-engine.test.ts`:
- Around line 165-182: The test "skips nodes matching excludeNodeNames pattern"
doesn't prove exclusion because it never verifies the ignored node produced
issues before applying excludeNodeNames; update the test to first run
analyzeFile(file) without the exclude option and assert there are one or more
issues for node id "i:1" (using makeNode/makeFile and result.issues filtering),
then run analyzeFile(file, { excludeNodeNames: ["IgnoreMe"] }) and assert the
filtered ignoredIssues length is 0 to demonstrate the exclusion behavior.
- Around line 188-206: The test "runs only enabledRules when specified" can pass
vacuously when result.issues is empty; update the assertion to first assert
there is at least one issue (e.g.,
expect(result.issues.length).toBeGreaterThan(0) or similar) before iterating,
then keep the existing loop that ensures every issue.violation.ruleId ===
"default-name" using analyzeFile, enabledRules and result.issues to locate the
code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ea6ecaf-4ff2-431d-b731-cd6e963010d4
📒 Files selected for processing (1)
src/core/engine/rule-engine.test.ts
- excludeNodeNames: add baseline run to prove node generates issues before verifying exclusion filters them out - enabledRules: assert issues.length > 0 before loop to prevent vacuous pass on empty result - no-dev-status: add positive control — explicitly enable the rule to prove it fires, then verify default config disables it Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
let-sunny
added a commit
that referenced
this pull request
Mar 25, 2026
Add 5 test files with 37 new tests (427 → 464 total): Integration test (integration.test.ts): - Full pipeline: fixture load → analyzeFile → calculateScores → buildResultJson - targetNodeId scoping, score structure validation, deterministic output High-score rule unit tests (previously untested): - no-auto-layout (-7, blocking): 6 tests - absolute-position-in-auto-layout (-10, blocking): 7 tests - raw-color (-8, risk): 6 tests - ambiguous-structure (-10, blocking): 6 tests Addresses #50 (CLI integration test + high-score rule tests per comment consensus). scoring.ts and rule-engine.ts unit tests were already added in #58. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks
let-sunny
added a commit
that referenced
this pull request
Mar 25, 2026
* test: add integration test and high-score rule unit tests (#50) Add 5 test files with 37 new tests (427 → 464 total): Integration test (integration.test.ts): - Full pipeline: fixture load → analyzeFile → calculateScores → buildResultJson - targetNodeId scoping, score structure validation, deterministic output High-score rule unit tests (previously untested): - no-auto-layout (-7, blocking): 6 tests - absolute-position-in-auto-layout (-10, blocking): 7 tests - raw-color (-8, risk): 6 tests - ambiguous-structure (-10, blocking): 6 tests Addresses #50 (CLI integration test + high-score rule tests per comment consensus). scoring.ts and rule-engine.ts unit tests were already added in #58. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add raw-font rule unit tests Made-with: Cursor * test: add unit tests for all 27 remaining untested rules Cover every rule across all 6 categories: - layout: group-usage, missing-responsive-behavior, fixed-width-in-responsive-context, overflow-hidden-abuse, inconsistent-sibling-layout-direction - token: magic-number-spacing, raw-shadow, inconsistent-spacing, raw-opacity, multiple-fill-colors - component: detached-instance, variant-not-used, component-property-unused, single-use-component - naming: default-name, non-semantic-name, inconsistent-naming-convention, numeric-suffix-name, too-long-name - ai-readability: z-index-dependent-layout, missing-layout-hint, empty-frame - handoff-risk: hardcode-risk, text-truncation-unhandled, image-no-placeholder, prototype-link-in-design, no-dev-status Rule test coverage: 12/39 → 39/39 (100%) Total: 28 → 61 test files, 427 → 641 tests Made-with: Cursor * fix: internal calibration command edge cases (#63) - calibrate-evaluate: make positional args optional (--run-dir makes them unnecessary) - calibrate-run: remove unused --output option (was passing literal "unused") - calibrate-gap-report: fix --min-repeat 0 edge case (parseInt || 2 treated 0 as falsy) - calibrate-prune-evidence: use stderr + non-zero exit code on errors Addresses #63 items 1, 2, 3, 5. Item 4 (global registry mutation) deferred — requires ruleRegistry refactor. Made-with: Cursor * refactor: extract shared test builders + address review comments - Extract makeNode/makeFile/makeContext to shared test-helpers.ts - Update 33 rule test files to use shared builders (removes ~800 lines of duplication) - Fix integration.test.ts: use exact set assertion for categories - Fix ambiguous-structure.test.ts: remove explicit layoutMode: undefined (exactOptionalPropertyTypes) - Fix absolute-position.test.ts: remove explicit parent: undefined Addresses CodeRabbit review on PR #71. Made-with: Cursor * test: add edge case tests + convert stubs to it.todo Address CodeRabbit review round 2: - empty-frame: 48x48 boundary + asymmetric dimension tests - z-index-dependent-layout: exact 20% overlap boundary + missing bbox - missing-layout-hint: one-auto-layout-child edge case - single-use-component: deeply nested instance traversal - inconsistent-naming-convention: tie-case determinism - text-truncation-unhandled: exact 300px boundary - fixed-width-in-responsive-context: fallback path (no layoutSizingHorizontal) - inconsistent-sibling-layout-direction: single-sibling boundary - non-semantic-name: fix leaf-primitive test (was hitting excluded-name path) - numeric-suffix-name: empty name edge case - Convert 5 stub tests to it.todo (component-property-unused, variant-not-used, prototype-link-in-design, overflow-hidden-abuse, multiple-fill-colors) 652 tests (646 passed + 6 todo), lint clean. Made-with: Cursor * fix: treat missing run dir / debate as normal in calibrate-prune-evidence Prune command encountering no run dir or no debate.json is a normal outcome (nothing to prune), not an error. Changed console.error → console.log and removed process.exitCode = 1 so subagents continue working without false error signals. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review round 4 — test description, unused import, boundary tests - missing-layout-hint.test.ts: fix misleading test name ("flags" → "does not flag") - variant-not-used.test.ts: remove unused makeNode/makeFile/makeContext import - text-truncation-unhandled.test.ts: add boundary tests for characters.length 50/51 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
scoring.ts에 23개 유닛 테스트 신규 추가 (기존 0개)rule-engine.test.ts에 15개 테스트 추가 (기존 1개 → 16개)scoring.test.ts (신규, 23 tests)
calculateScores: zero issues→100%, severity 카운팅, density 가중치, diversity 패널티, combined 공식 검증, floor clamping, nodeCount 엣지 케이스 (0, 1)formatScoreSummary,getCategoryLabel,getSeverityLabelbuildResultJson필드 구조 + issuesByRule 집계 검증rule-engine.test.ts (확장, +15 tests)
targetNodeId: 서브트리 스코핑, dash→colon 정규화, 존재하지 않는 노드 에러excludeNodeTypes/excludeNodeNames필터링enabledRules/disabledRules/config.enabled필터링depthWeight보간 (root vs leaf 점수 차이)analyzeFileconvenience function + custom configsTest plan
pnpm test:run— 426 tests passedpnpm lint— 타입 체크 통과Closes #55
🤖 Generated with Claude Code
Summary by CodeRabbit